refactor: correct the implementation of all_schemas()
#5236
Conversation
jackwener
force-pushed
the
all_schema()
branch
from
5ab74a1
to
66dd1ad
Compare
There was a problem hiding this comment.
Thank you @jackwener
This function has aways seemed strange to me -- I wonder if we can work on getting rid of it entirely.
I think every Expr is evaluated in the context of a LogicalPlan and the schema within which to evaluate the Expr is clear:
(LogicalPlan) <-- Exprs *within* a logical plan should be interpreted
with respect to the *child output schemas*
(aka the inputs schemas)
I realize you didn't introduce this function, and this change seems to make things better (and all the tests pass) but I still think there is more work to do here somewhere.
Thanks again
|
Thanks again @jackwener |
|
Benchmark runs are scheduled for baseline = 4eb1a57 and contender = 0f95966. 0f95966 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #5192.
Rationale for this change
Currently, all_schema() can return schema of child of child. (due to recursively call
all_schema())But, plan shouldn't get schema of child of child (or child of child of child of child ......).
It should just get info of itself and children.
For example
join shouldn't get schema of
B.So, I refactor this implementation, remove recursion.
What changes are included in this PR?
Just return children schema and itself schema
Are these changes tested?
Are there any user-facing changes?